Skip to content

Bind SharedValues in handler config #3658

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 17 commits into
base: next
Choose a base branch
from
Open

Bind SharedValues in handler config #3658

wants to merge 17 commits into from

Conversation

m-bert
Copy link
Contributor

@m-bert m-bert commented Aug 5, 2025 •
edited
Loading

Description

This PR introduces SharedValue bindings to config. This will allow users to specify values in config asSharedValues, e.g:

const taps = useSharedValue(2);

const gesture = useGesture('TapGestureHandler', {
  onEnd: () => {
    console.log('Tap detected. Required number of taps:', taps.value);
    taps.set(taps.value + 1);
  },
  numberOfTaps: taps,
});

Test plan

Tested on the following code: 
import * as React from 'react';
import { Animated, Button } from 'react-native';
import { useSharedValue } from 'react-native-reanimated';
import {
  GestureHandlerRootView,
  NativeDetector,
  useGesture,
} from 'react-native-gesture-handler';

export default function App() {
  const [visible, setVisible] = React.useState(true);
  const taps = useSharedValue(2);

  const gesture = useGesture('TapGestureHandler', {
    onEnd: () => {
      console.log('Tap detected. Required number of taps:', taps.value);
      taps.set(taps.value + 1);
    },
    numberOfTaps: taps,
  });

  return (
    <GestureHandlerRootView
      style={{ flex: 1, backgroundColor: 'white', paddingTop: 8 }}>
      <Button
        title="Toggle visibility"
        onPress={() => {
          setVisible(!visible);
        }}
      />

      {visible && (
        <NativeDetector gesture={gesture}>
          <Animated.View
            style={[
              {
                width: 150,
                height: 150,
                backgroundColor: 'blue',
                opacity: 0.5,
                borderWidth: 10,
                borderColor: 'green',
                marginTop: 20,
                marginLeft: 40,
              },
            ]}
          />
        </NativeDetector>
      )}
    </GestureHandlerRootView>
  );
}

m-bert
m-bert commented Aug 6, 2025
View reviewed changes
packages/react-native-gesture-handler/apple/RNGestureHandlerModule.mm
Comment on lines +187 to +188
RNGestureHandlerManager *manager = [RNGestureHandlerModule handlerManagerForModuleId:_moduleId];
[manager updateGestureHandlerConfig:[NSNumber numberWithDouble:handlerTag] config:config];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if this should be done like this, or like in setGestureHandlerConfig. However, if we use addOperationBlock we have to call flushOperations, so I think we can leave it as it is (cc @j-piasecki)

@m-bert m-bert marked this pull request as ready for review August 6, 2025 10:10
@m-bert m-bert requested review from tomekzaw and j-piasecki August 6, 2025 10:10
@m-bert

This comment was marked as outdated.

Sign in to view
j-piasecki
j-piasecki requested changes Aug 8, 2025
View reviewed changes
...e-handler/android/src/main/java/com/swmansion/gesturehandler/react/RNGestureHandlerModule.kt Outdated
Comment on lines 112 to 122
override fun setGestureHandlerConfig(handlerTagDouble: Double, config: ReadableMap) {
val handlerTag = handlerTagDouble.toInt()

updateGestureHandlerHelper<GestureHandler>(handlerTag, config, false)
}

@ReactMethod
override fun updateGestureHandlerConfig(handlerTagDouble: Double, config: ReadableMap) {
val handlerTag = handlerTagDouble.toInt()

updateGestureHandlerHelper<GestureHandler>(handlerTag, config)
updateGestureHandlerHelper<GestureHandler>(handlerTag, config, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think updateGestureHandlerHelper can be inlined in both places. Now that handlers are no longer generic classes, the helper doesn't add much, and the result should be easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, done in 50ffbcd

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated
RNGestureHandlerModule.dropGestureHandler(tag);
RNGestureHandlerModule.flushOperations();
};
}, [type, tag]);
}, [type, tag, config]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause the gesture to be dropped when the config object is changed. Shared value binding should be handled by a separate effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, changed in 8d1ad9b

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated
Comment on lines 52 to 55
const sharedValues = new Map<
string,
{ id: number; sharedValue: SharedValue }
>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's a need to store shared values in a map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 2af12c1

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated
{ id: number; sharedValue: SharedValue }
>();

let nextSharedValueListenerID = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reanimated also numbers from 0, we need some kind of offset - pick your favorite, but not obvious (rational or rounded irrational) number. We shouldn't need that counter anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reanimated also numbers from 0

Reanimated starts from 9999 😅 But I do agree that we may want to use offset.

Also, do we need to use tag + offset? If each shared value has its own map of listeners, it should be fine to use just offset and then we could simply call it listenerId.

Changed in 9a98e19, though this commit has SharedValues in array. It was removed later on in 2af12c1.

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated
Comment on lines 84 to 87
sharedValues.set(key, {
id: nextSharedValueListenerID,
sharedValue: maybeSharedValue,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sharedValues.set(key, {
id: nextSharedValueListenerID,
sharedValue: maybeSharedValue,
});

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated

Reanimated.runOnUI(attachListener)(
maybeSharedValue,
nextSharedValueListenerID,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nextSharedValueListenerID,

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated
key
);

nextSharedValueListenerID++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
nextSharedValueListenerID++;

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated
Comment on lines 106 to 112
for (const [key, { id, sharedValue }] of sharedValues.entries()) {
Reanimated.runOnUI(() => {
sharedValue.removeListener(id);
})();

sharedValues.delete(key);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (const [key, { id, sharedValue }] of sharedValues.entries()) {
Reanimated.runOnUI(() => {
sharedValue.removeListener(id);
})();
sharedValues.delete(key);
}
for (const [key, maybeSharedValue] of Object.entries(config)) {
if (!Reanimated.isSharedValue(maybeSharedValue)) {
continue;
}
Reanimated.runOnUI(() => {
maybeSharedValue.removeListener(handlerTag + MAGIC_OFFSET);
})();
}

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated
// This is used to obtain HostFunction that can be executed on the UI thread
const { updateGestureHandlerConfig } = RNGestureHandlerModule;

function maybeExtractSharedValues(config: any, tag: number) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function maybeExtractSharedValues(config: any, tag: number) {
function bindSharedValues(config: any, tag: number) {

Imo, we don't need maybe prefix here. If there's no Reanimated, there will be no shared values - every shared value would be bound. Same, if there's Reanimated but no shared values in the config. And if there's Reanimated and shared values, they will be bound correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done in fd6eb79

packages/react-native-gesture-handler/src/v3/hooks/useGesture.ts Outdated
}
}

function maybeDetachSharedValues() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function maybeDetachSharedValues() {
function unbindSharedValues() {

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in fd6eb79

@m-bert m-bert requested a review from j-piasecki August 11, 2025 11:25
@akwasniewski akwasniewski mentioned this pull request Aug 12, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomekzaw tomekzaw Awaiting requested review from tomekzaw

@j-piasecki j-piasecki Awaiting requested review from j-piasecki

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@m-bert @j-piasecki